Skip to content

feat(storage): store push-publishing bundles durably in S3 asset storage - #37774

Open
swicken wants to merge 4 commits into
s3-stack/3-recoveryfrom
s3-stack/4-publishing
Open

feat(storage): store push-publishing bundles durably in S3 asset storage#37774
swicken wants to merge 4 commits into
s3-stack/3-recoveryfrom
s3-stack/4-publishing

Conversation

@swicken

@swicken swicken commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

S3 asset storage, part 5 of 7. Stacked PRs, review bottom up. Each one builds on the one below.
1a storage layer #37770 · 1b binary asset API #37771 · 2 content #37772 · 3 recovery #37773 · 4 publishing #37774 · 5 temporary uploads and WebDAV #37775 · 6 rendering #37776
Everything is behind FEATURE_FLAG_S3_ASSET_STORAGE, off by default. With the flag off, behavior matches main.

Rebased on 2026-10-06 onto a fix in #37772 (3523198625). This PR's own commits are unchanged; the test counts below were taken before that rebase.

Refs #37868

Proposed Changes

  • BundleArchiveStorage stores completed push-publishing archives (<bundle-id>.tar.gz, manifest included) in the publishing-bundles group, with the bundle directory as a cache. Bundle ids keep their case and cannot resolve outside the bundle directory. Stored and received archives use the same id rule the receivers use (the name up to the first .tar.gz).
  • Generated and received bundles (BundlePublisherResource, BundleResource, RemotePublishAjaxAction, TarGzipBundleOutput) are staged privately and published to S3 before they replace the last complete archive, so a partial upload never becomes visible. Manifest and payload reads restore the archive only when needed.
  • Retention. BinaryCleanupJob expires the S3 copies after the same CLEANUP_BUNDLES_OLDER_THAN_DAYS (default 4) as the local bundle directory.
  • Committed bundle deletion records a bundleArchiveCleanup job, which keeps the local bytes if remote deletion fails. If the bundle's row exists again when the job runs, because the bundle was received again, the job ends as a no-op.
  • The bundle list and audit pages use a display-only existence check that logs a storage error and shows the bundle as not generated, so the pages still render when S3 is down. Publishing and retry decisions keep the strict check.

Behavior with the flag off

Unchanged from main. The bundle path is built the same way (only the File construction differs), and the new path checks apply only with the flag on. The bundle pages now evaluate the existence check once per bundle instead of up to three times, with the same result. No REST annotations change, so openapi.yaml is unchanged.

Review fixes

The last commit on this branch (fix(storage): bound durable bundle retention and harden flag-on bundle paths) addresses a full review of this PR. All of it is flag-on only:

  • S3 bundle archives used to be kept forever; they now expire with the local ones.
  • The bundle pages no longer make an S3 request per row on every check, and no longer fail to render when S3 is unavailable.
  • Static publishing fails with a retryable error when a file asset's binary is missing, as it does with the flag off, instead of silently publishing without the file. The cache lease is held across the lookup and the copy.
  • Store and read derive the bundle id the same way.
  • The cleanup job's check-then-delete is closed against a re-received bundle on the same node, by a per-bundle lock held by the store and by the job. Two narrow windows remain and are documented: another cluster node, and a receive whose row has not committed yet. The worst case is a failed receive that the sender retries, with no data loss.
  • An S3 error during download cleanup is caught and logged instead of escaping the handler.

Checklist

  • Tests: the 174 unit tests in the doc's run command pass after the review fixes (1 skip, STS), including the new BundleArchiveStorageTest and FileAssetBundlerTest. Integration on this branch before the review fixes, flag off: 75 run, 0 failures, across the new PublishingArchiveStorageTest and eight existing publishing tests (PublisherAPIImplTest x2, BundlePublisherTest, BundlerUtilTest, BundlerUtilIntegrationTest, ManifestReaderFactoryTest, ManifestUtilTest, BundleResourceTest, PushPublishBundleGeneratorTest); flag on: 21 run, 0 failures. After the review fixes, PublishingArchiveStorageTest and BundleResourceTest passed flag off and on in the run on the top of the stack (feat(storage): serve renditions, compiled CSS and templates through S3 asset storage #37776).
  • Translations: not applicable.
  • Security Implications Contemplated: with the flag on, bundle ids are validated and must resolve inside the bundle directory before any path is used, and received uploads are written to a separate staging file.

This was referenced Sep 28, 2026
Open
Open
Open
Open
Open
Open
@claude

claude Bot commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @jcastro-dotcms's task in 4m 1s —— View job

Review: durable S3 push-publishing bundles (#37774)

  • Read core new/changed files
  • Analyze bugs, security, error paths, replay safety
  • Recheck prior rollback-safety findings
  • Post review

This is a careful, well-staged change. The private-staging-then-publish pattern (TarGzipBundleOutput.create()→complete(), BundleArchiveStorage.receive()) correctly prevents a partial archive from ever becoming visible; complete() closes and nulls the stream before close() runs, so there's no double-close and the temp is always cleaned up. Path handling in localArchive() and the delete()/ManifestUtil canonical-path checks block traversal, bundle ids keep their case via normalizePath, and the SQL in the cleanup job uses addParam(). The strict (exists) vs display (existsForDisplay) split is applied consistently — publish/retry use the throwing check, the JSPs and audit page use the logged-and-false check — and the JSPs now evaluate it once per row. Flag-off paths are untouched.

New Issues

  • 🟡 Medium: dotCMS/src/main/java/com/dotcms/publishing/output/BundleArchiveCleanupProcessor.java:49-51 — If FEATURE_FLAG_S3_ASSET_STORAGE is toggled off after a bundle delete has already enqueued a bundleArchiveCleanup job (the enqueue happened while it was on), process() throws JobProcessingException on every attempt and can never succeed while the flag is off. The job exhausts its retries and the S3 archive is orphaned — BinaryCleanupJob.expireDurableBundles also no-ops when the flag is off (BundleArchiveStorage.expireOlderThan returns 0), so nothing reclaims it until the flag is turned back on. No data loss, but worth considering an idempotent no-op/skip (as the "bundle exists again" branch already does) rather than a hard failure for the flag-off case. Assumption: operators may disable the flag with in-flight cleanup jobs queued. What to verify: retry-policy terminal behavior and whether an operator ever expects to disable the flag after enabling it.

Prior findings (rollback-safety bot comments)

  • The two 🟠 H-5 "unsafe to rollback" notices are accurate and are not code bugs — they're the inherent forward-only consequence of treating the local bundle directory as a cache in front of S3. This PR documents exactly that in docs/testing/BINARY_S3_STORAGE.md ("## Publishing bundles" + the pre-existing "Enabling the flag is not rollback-safe" warning), consistent with how binary assets are already handled. No change required beyond the operational awareness the doc already provides.

Notes (non-blocking)

  • getBundleTarGzipFile() now resolves via BundleArchiveStorage.get(), which does a full pullFile (S3 download) on every call. In the existence-check callers (PublishingRetryHelper.verifyBundle:286, RemotePublishAjaxAction:413) this downloads the whole archive just to test .exists(). It's not wasted — those paths go on to read the bundle, so the download primes the cache — but if any future caller only needs presence, prefer BundleArchiveStorage.exists().

Nothing here is blocking. The one Medium is a narrow flag-toggle edge with no data-loss impact.

· s3-stack/4-publishing

@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Pull Request Unsafe to Rollback!!!

  • Category: H-5 — Binary/Storage Provider Configuration Change
  • Risk Level: 🟠 HIGH
  • Why it's unsafe: This PR wires push-publishing bundle archives (<bundle-id>.tar.gz + manifest) into the existing FEATURE_FLAG_S3_ASSET_STORAGE opt-in storage chain, which docs/testing/BINARY_S3_STORAGE.md already documents (pre-existing text, unchanged by this PR) as "Enabling the flag is not rollback-safe." BundleArchiveStorage (new class) treats the local bundle directory as merely a cache in front of S3 — the doc's new "## Publishing bundles" section states archives are "stored durably in the publishing-bundles group, with the local bundle directory as a cache." BundleArchiveStorage.store()/.get() (BundleArchiveStorage.java:104-137) dual-write to local FS + S3 at creation/receive time, but that local copy is not durable across node/pod lifecycle: in a container-orchestrated rollback (redeploy to the N-1 image), the new N-1 pod starts with an empty/ephemeral local bundle directory. N-1 has no knowledge of BundleArchiveStorage/S3 and only ever does plain File I/O against the conventional path (e.g. PublishingRetryHelper.java, BundlePublisher.java, RemotePublishAjaxAction.java before this diff). Any bundle whose bytes only live in S3 relative to that fresh pod (i.e., not re-pulled onto that specific node) becomes unreadable after rollback — bundleFile.exists() returns false, and push-publish retry/receive/download flows fail with "No Bundle Found" / "not found" errors. The integration test added in this PR (PublishingArchiveStorageTest.java:74-78, assertRemoteCopyAndClearLocal + assertFalse(archive.exists(), "Listing a bundle must not download its archive")) itself demonstrates that the local file is expected to go missing while only the S3 copy remains authoritative — exactly the condition N-1 cannot handle.
  • Code that makes it unsafe: dotCMS/src/main/java/com/dotcms/publishing/output/BundleArchiveStorage.java (new file, the whole class), dotCMS/src/main/java/com/dotcms/publishing/output/TarGzipBundleOutput.java (getBundleTarGzipFile now delegates to BundleArchiveStorage.getInstance().get(bundleId)), dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java (adds BundleArchiveCleanupProcessor to the shared FEATURE_FLAG_S3_ASSET_STORAGE lifecycle), docs/testing/BINARY_S3_STORAGE.md ("## Publishing bundles" section + the pre-existing, still-applicable "Enabling the flag is not rollback-safe" section).
  • Alternative (if possible): Per H-5's safer alternative, guarantee the legacy local path is always populated and never treated as merely a cache for this data type — i.e., never evict/rely-on-S3-only for bundle archives, or explicitly extend the existing "not rollback-safe" doc warning to call out push-publishing bundles by name so operators know that enabling the flag also makes bundle rollback forward-only, consistent with how binary assets are already documented.

@swicken
swicken force-pushed the s3-stack/4-publishing branch from 2da5728 to 4701296 Compare September 28, 2026 19:32
@swicken
swicken force-pushed the s3-stack/3-recovery branch from 05b261e to 4ae1a5c Compare September 28, 2026 19:32
@swicken
swicken force-pushed the s3-stack/4-publishing branch from 4701296 to fc1d9d5 Compare September 29, 2026 14:26
@swicken
swicken force-pushed the s3-stack/3-recovery branch from 4ae1a5c to 86992b7 Compare September 29, 2026 14:26
@swicken
swicken force-pushed the s3-stack/4-publishing branch from fc1d9d5 to a0e9eab Compare September 29, 2026 19:32
@swicken
swicken force-pushed the s3-stack/3-recovery branch from 86992b7 to 4f778fe Compare September 29, 2026 19:32
@swicken swicken mentioned this pull request Oct 2, 2026
Open
3 of 19 tasks
@swicken
swicken force-pushed the s3-stack/3-recovery branch from 4f778fe to f552bdb Compare October 2, 2026 15:54
@swicken
swicken force-pushed the s3-stack/4-publishing branch 2 times, most recently from 2a60b57 to 6964d89 Compare October 2, 2026 16:31
@swicken
swicken force-pushed the s3-stack/3-recovery branch from f552bdb to 13879e9 Compare October 2, 2026 16:31
@swicken
swicken marked this pull request as ready for review October 2, 2026 18:24
@swicken
swicken force-pushed the s3-stack/4-publishing branch from 6964d89 to 788f4d0 Compare October 6, 2026 14:43
@swicken
swicken force-pushed the s3-stack/3-recovery branch from 13879e9 to 6771ed3 Compare October 6, 2026 14:43
@nollymar nollymar added the PR : dotbot review Trigger dotbot AI code review and the post-merge QA test plan label Oct 6, 2026
swicken added 3 commits October 7, 2026 16:25
@swicken
feat(storage): store push-publishing bundles durably in S3 asset storage
3dab9e9 
Fifth slice of the S3 asset storage work. With FEATURE_FLAG_S3_ASSET_STORAGE on,
generated and received publishing bundles are staged privately and published to
the publishing-bundles S3 group before replacing the local archive, reads restore
archives on demand, and committed bundle deletion schedules a retrying
bundleArchiveCleanup job. The cleanup processor does not register while the flag
is off, and flag-off bundle handling matches main.
@swicken
refactor(storage): take the publishing bundle remote store from the s…
1c9e448 
…torage provider
@swicken
fix(storage): bound durable bundle retention and harden flag-on bundl…
4f3af4b 
…e paths

These fixes apply only with FEATURE_FLAG_S3_ASSET_STORAGE on; flag-off bundle paths are unchanged.

Durable bundle archives now age out. BinaryCleanupJob.cleanUpOldBundles calls a new
expireDurableBundles method, which asks BundleArchiveStorage.expireOlderThan to delete S3 archives
whose last-modified time is older than CLEANUP_BUNDLES_OLDER_THAN_DAYS, the same threshold the
local bundle directory cleanup uses. Previously the S3 copies were only removed when a user deleted
bundle history.

The bundle pages no longer depend on S3 being reachable. Bundle.bundleTgzExists and the
retry-button check in BundlerUtil use a new BundleArchiveStorage.existsForDisplay, which logs a
storage failure and reports the archive as missing, while exists stays strict for publishing and
retry decisions. view_unpushed_bundles.jsp and edit_publish_bundle.jsp check once per bundle
instead of up to three times.

Static publishing fails the bundle with an IOException when a File Asset's binary is missing,
matching the flag-off failure, instead of skipping the file and reporting success. The cache lease
is held across finding and copying the binary, and the log text with the wrong wording is gone.

BundleArchiveStorage.receive derives the bundle id from the part of the file name before the first
.tar.gz, the rule the receiving endpoints and BundlePublisher use, so names such as
release.tar.gz-v2.tar.gz or x.tar.gzip are stored where they are read.

The bundleArchiveCleanup job treats a bundle row that exists again as a successful no-op instead of
a retryable failure, and holds a per-bundle lock, also held by BundleArchiveStorage.store, across
its row check and delete.

The download cleanup in RemotePublishAjaxAction now also catches unchecked storage exceptions.

Adds BundleArchiveStorageTest and FileAssetBundlerTest, and documents retention, the page check,
the id rule and the remaining cleanup race in BINARY_S3_STORAGE.md.
@swicken
swicken force-pushed the s3-stack/3-recovery branch from 6771ed3 to ed86935 Compare October 7, 2026 20:47
@swicken
swicken force-pushed the s3-stack/4-publishing branch from 788f4d0 to 4f3af4b Compare October 7, 2026 20:47
@claude

claude Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Pull Request Unsafe to Rollback!!!

  • Category: H-5 — Binary / Storage Provider Change (adapted to push-publishing bundle archives)
  • Risk Level: 🟠 HIGH
  • Why it's unsafe: When FEATURE_FLAG_S3_ASSET_STORAGE is on, completed push-publishing archives are now stored durably in S3's publishing-bundles group, with the local bundle directory acting only as a cache (new BundleArchiveStorage). N-1 (the pre-PR binary) has no concept of BundleArchiveStorage or the publishing-bundles S3 group — it only knows how to read/write <bundle-id>.tar.gz directly under ConfigUtils.getBundlePath(). After a rollback, any archive whose bytes live primarily in S3 (generated/received on a different cluster node, or evicted/never cached locally on this node) becomes invisible to N-1: downloads, retries, and the sender/receiver code paths (RemotePublishAjaxAction, BundleResource, PublishingRetryHelper, BundlerUtil.tarGzipExists) will report the bundle as missing even though it still exists in S3. This is the same forward-only storage model that docs/testing/BINARY_S3_STORAGE.md already documents as 'not rollback-safe' for binary assets — this PR extends that identical risk to push-publishing bundles (see the PR's own new '## Publishing bundles' section in that same doc).
  • Code that makes it unsafe: dotCMS/src/main/java/com/dotcms/publishing/output/BundleArchiveStorage.java (new — store(), receive(), get(), delete(), expireOlderThan() route archives through the S3-backed chain with local disk as cache only); dotCMS/src/main/java/com/dotcms/publishing/output/TarGzipBundleOutput.java (getBundleTarGzipFile() now resolves via BundleArchiveStorage.getInstance().get(bundleId)); dotCMS/src/main/java/com/dotcms/publishing/BundlerUtil.java (tarGzipExists()/bundleExists() now check BundleArchiveStorage when the flag is on).
  • Alternative (if possible): The flag is opt-in and off by default, so most deployments are unaffected on rollback. For deployments that enable FEATURE_FLAG_S3_ASSET_STORAGE, treat it as the PR's own doc already recommends for binaries — a forward-only operational decision — and call out in release notes that rolling a node back after this flag has been enabled is unsafe for any push-publishing bundle generated or received while it was on.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

AI: Safe To Rollback PR : dotbot review Trigger dotbot AI code review and the post-merge QA test plan

Projects

dotCMS - Product Planning
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@swicken @nollymar @jcastro-dotcms